From: kaf24@firebug.cl.cam.ac.uk Date: Tue, 16 May 2006 13:07:56 +0000 (+0100) Subject: Fixes to the xenoprofile Linux driver. X-Git-Tag: archive/raspbian/4.8.0-1+rpi1~1^2~16047^2~61 X-Git-Url: https://dgit.raspbian.org/%22http://www.example.com/cgi/success//%22http:/www.example.com/cgi/success/?a=commitdiff_plain;h=eca207adce3e1b0f7aadb9caf1751c93adcaa1ca;p=xen.git Fixes to the xenoprofile Linux driver. The active_domains code has race conditions: * oprofile_set_active() calls set_active() method without holding start_sem. This is clearly wrong, as xenoprof_set_active() makes several hypercalls. oprofile_start(), for instance, could run in the middle of xenoprof_set_active(). * adomain_write(), adomain_read() and xenoprof_set_active() access global active_domains[] and adomains without synchronization. I went for a simple, obvious fix and created another mutex. Instead, one could move the shared data into oprof.c and protect it with start_sem, but that's more invasive. Also clean up the code dealing with /dev/oprofile/active_domains: * Use parameters instead of global variables to pass domain ids around. Give those globals internal linkage. * Allocate buffers dynamically to conserve stack space. * Treat writes with size zero exactly like a write containing no domain id. Before, zero-sized write was ignored, which is not the same. * Parse domain ids as unsigned numbers. Before, the first one was parsed as signed number. Because ispunct()-punctuation is ignored between domain ids, signs are still silently ignored except for the first number. Hmm. * Make parser accept whitespace as domain separator, because that's what you get when reading the file. * EINVAL on domain ids overflowing domid_t. Before, they were silently truncated. * EINVAL on too many domain ids. Before, the excess ones were silently ignored. * Reset active domains on failure halfway through setting them. * Fix potential buffer overflow in adomain_read(). Couldn't really happen because buffer was sufficient for current value of MAX_OPROF_DOMAINS. Signed-off-by: Markus Armbruster --- diff --git a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c index 5365ed6fcc..80323cd2d1 100644 --- a/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c +++ b/linux-2.6-xen-sparse/arch/i386/oprofile/xenoprof.c @@ -289,9 +289,13 @@ static int xenoprof_set_active(int * active_domains, for (i=0; i TMPBUFSIZE - 1) + return -EINVAL; + -+ memset(tmpbuf, 0x0, TMPBUFSIZE); ++ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL))) ++ return -ENOMEM; + -+ if (copy_from_user(tmpbuf, buf, count)) ++ if (copy_from_user(tmpbuf, buf, count)) { ++ kfree(tmpbuf); + return -EFAULT; -+ -+ for (i = 0; i < MAX_OPROF_DOMAINS; i++) -+ active_domains[i] = -1; -+ adomains = 0; ++ } ++ tmpbuf[count] = 0; + -+ while (1) { -+ val = simple_strtol(startp, &endp, 0); ++ mutex_lock(&adom_mutex); ++ ++ startp = tmpbuf; ++ /* Parse one more than MAX_OPROF_DOMAINS, for easy error checking */ ++ for (i = 0; i <= MAX_OPROF_DOMAINS; i++) { ++ val = simple_strtoul(startp, &endp, 0); + if (endp == startp) + break; -+ while (ispunct(*endp)) ++ while (ispunct(*endp) || isspace(*endp)) + endp++; -+ active_domains[adomains++] = val; -+ if (adomains >= MAX_OPROF_DOMAINS) -+ break; ++ active_domains[i] = val; ++ if (active_domains[i] != val) ++ /* Overflow, force error below */ ++ i = MAX_OPROF_DOMAINS + 1; + startp = endp; + } -+ if (oprofile_set_active()) -+ return -EINVAL; -+ return count; ++ /* Force error on trailing junk */ ++ adomains = *startp ? MAX_OPROF_DOMAINS + 1 : i; ++ ++ kfree(tmpbuf); ++ ++ if (adomains > MAX_OPROF_DOMAINS ++ || oprofile_set_active(active_domains, adomains)) { ++ adomains = 0; ++ retval = -EINVAL; ++ } ++ ++ mutex_unlock(&adom_mutex); ++ return retval; +} + +static ssize_t adomain_read(struct file * file, char __user * buf, + size_t count, loff_t * offset) +{ -+ char tmpbuf[TMPBUFSIZE]; -+ size_t len = 0; ++ char * tmpbuf; ++ size_t len; + int i; -+ /* This is all screwed up if we run out of space */ -+ for (i = 0; i < adomains; i++) -+ len += snprintf(tmpbuf + len, TMPBUFSIZE - len, -+ "%u ", (unsigned int)active_domains[i]); -+ len += snprintf(tmpbuf + len, TMPBUFSIZE - len, "\n"); -+ return simple_read_from_buffer((void __user *)buf, count, -+ offset, tmpbuf, len); ++ ssize_t retval; ++ ++ if (!(tmpbuf = kmalloc(TMPBUFSIZE, GFP_KERNEL))) ++ return -ENOMEM; ++ ++ mutex_lock(&adom_mutex); ++ ++ len = 0; ++ for (i = 0; i < adomains; i++) ++ len += snprintf(tmpbuf + len, ++ len < TMPBUFSIZE ? TMPBUFSIZE - len : 0, ++ "%u ", active_domains[i]); ++ WARN_ON(len > TMPBUFSIZE); ++ if (len != 0 && len <= TMPBUFSIZE) ++ tmpbuf[len-1] = '\n'; ++ ++ mutex_unlock(&adom_mutex); ++ ++ retval = simple_read_from_buffer(buf, count, offset, tmpbuf, len); ++ ++ kfree(tmpbuf); ++ return retval; +} + +